Add labels to VMs #516

darkowlzz · 2020-02-02T14:40:31Z

Adds a new flag --label to VM create command for setting label on the VM
object. The flag is of StringArray type and can be used multiple times
to set more than one label.

# ignite create weaveworks/ignite-ubuntu --name my-vm1 -l label1=val1 -l label2=val2

vm inspect result:

# ignite inspect vm my-vm1
{
  "kind": "VM",
  "apiVersion": "ignite.weave.works/v1alpha2",
  "metadata": {
    "name": "my-vm1",
    "uid": "70d265ddaaa92462",
    "created": "2020-02-02T12:37:21Z",
    "labels": {
      "label1": "val1",
      "label2": "val2"
    }
...

chanwit

Thank you for submitting this PR, @darkowlzz :-)
Not sure if you think my comment to change Split to SplitN is making sense?

Other than this small nit, it LGTM

chanwit · 2020-02-02T16:53:41Z

pkg/metadata/metadata.go

+	}
+
+	for _, label := range labels {
+		kv := strings.Split(label, "=")


should we use SplitN(label, "=", 2), wdyt?
if so, the value could also be possible to contain other = chars too.

Yes, that's a good idea. I checked what happens if I pass "k1=v1,k2=v2" as label to docker, and it splits at first separator and accepts the rest of the string as the value.

"Labels": { "k1": "v1,k2=v2" }

Updating the test to not fail for this case.
Thanks!

Adds a new flag --label to VM create command for setting label on the VM object. The flag is of StringArray type and can be used multiple times to set more than one label. ``` $ ignite create weaveworks/ignite-ubuntu --name vm -l k1=v1 -l k2=v2 ```

darkowlzz · 2020-02-03T09:09:47Z

Added another check to ensure that the label name isn't empty. Updated the test with this case.

chanwit

LGTM

chanwit · 2020-02-03T11:05:04Z

Thank you @darkowlzz for this wonderful PR!!

darkowlzz requested a review from twelho as a code owner February 2, 2020 14:40

chanwit requested review from chanwit and removed request for twelho February 2, 2020 16:47

chanwit suggested changes Feb 2, 2020

View reviewed changes

chanwit added the kind/enhancement Categorizes issue or PR as related to improving an existing feature. label Feb 2, 2020

chanwit self-assigned this Feb 2, 2020

darkowlzz force-pushed the vm-labels branch from 7bab35b to 6df70be Compare February 3, 2020 08:53

Add labels to VMs

626cf79

Adds a new flag --label to VM create command for setting label on the VM object. The flag is of StringArray type and can be used multiple times to set more than one label. ``` $ ignite create weaveworks/ignite-ubuntu --name vm -l k1=v1 -l k2=v2 ```

darkowlzz force-pushed the vm-labels branch from 6df70be to 626cf79 Compare February 3, 2020 09:08

chanwit approved these changes Feb 3, 2020

View reviewed changes

chanwit merged commit b88dce6 into weaveworks:master Feb 3, 2020

darkowlzz deleted the vm-labels branch February 3, 2020 11:04

luxas added this to the v0.7.0 milestone Jun 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add labels to VMs #516

Add labels to VMs #516

darkowlzz commented Feb 2, 2020 •

edited

Loading

chanwit left a comment •

edited

Loading

chanwit Feb 2, 2020

darkowlzz Feb 3, 2020 •

edited

Loading

darkowlzz commented Feb 3, 2020

chanwit left a comment

chanwit commented Feb 3, 2020

Add labels to VMs #516

Add labels to VMs #516

Conversation

darkowlzz commented Feb 2, 2020 • edited Loading

chanwit left a comment • edited Loading

Choose a reason for hiding this comment

chanwit Feb 2, 2020

Choose a reason for hiding this comment

darkowlzz Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

darkowlzz commented Feb 3, 2020

chanwit left a comment

Choose a reason for hiding this comment

chanwit commented Feb 3, 2020

darkowlzz commented Feb 2, 2020 •

edited

Loading

chanwit left a comment •

edited

Loading

darkowlzz Feb 3, 2020 •

edited

Loading